chore(agents): add verbatim dd-apm-sdk-review skill copy - #12459
robertomonteromiguel wants to merge 9 commits into
Conversation
Mirror the shared skill plus Claude/Cursor links. Repo-specific overrides and the perf-review replacement land in the follow-up.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Bring in the portable gh --repo pin, optional repo-context, and .claude/.cursor instruction-only shortcut without tracer-specific edits.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Load the data-not-instructions rule before repo-context.md.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Scan PR title, labels, and commit subjects before printing, using the same SECRET_GREP path as the diffs.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The ownership rule covers Cursor content. The ignore rule excludes local Cursor data and allows the shared skill link.
🤖 Datadog Autotest · Commit 745e7a3 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
The "Check pull requests" status check is failing: "Please add at least one type, and one component or instrumentation label to the pull request." Per this repo's conventions, please add:
|
robertomonteromiguel
added
type: feature
Verbatim copy: binary secret-scan fail-closed, LC_ALL=C git diff, and private routing for no-harness security findings.
Drop the security lens; local review is on-demand.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a30c636f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,338 @@ | |||
| --- | |||
| name: dd-apm-sdk-review | |||
| description: "On-demand multi-perspective read-only review of changes in this tracer repo, consolidated into one report with an explicit go / no-go verdict. Invoke when asked. GitHub Codex does not use this file — it follows review-without-harness.md via AGENTS.md." | |||
There was a problem hiding this comment.
When GitHub Codex performs the review, this description says it discovers review-without-harness.md through AGENTS.md, but this commit leaves the root AGENTS.md unchanged and a repo-wide search finds no reference to either the contract or this skill outside the new skill directory. Consequently, @codex review continues using the generic review instructions and never loads these new lenses; add the promised root AGENTS.md wiring.
Useful? React with 👍 / 👎.
| git rev-parse --is-shallow-repository # if true, merge-base may not resolve | ||
| if BASE=$(git merge-base HEAD "$TARGET" 2>/dev/null) && [ -n "$BASE" ]; then | ||
| emit_diff_or_redact "committed stat $BASE...HEAD" git diff --stat "$BASE"...HEAD | ||
| emit_diff_or_redact "committed $BASE...HEAD" git diff "$BASE"...HEAD |
There was a problem hiding this comment.
When a developer has diff.external or a diff.<driver>.textconv helper configured, these plain git diff calls execute that helper, allowing it to replace or suppress the patch handed to reviewers and potentially run mutating commands during this read-only workflow. This was reproduced locally, and git diff --no-index -h documents --[no-]ext-diff as allowing an external diff helper to execute; pass --no-ext-diff --no-textconv on the committed, staged, and unstaged diff calls.
Useful? React with 👍 / 👎.
| if [ -L "./$f" ] && [ -d "./$f" ]; then | ||
| echo "skipping untracked symlink-to-directory: $f (not scanning the target)" | ||
| continue |
There was a problem hiding this comment.
When an untracked symlink points to an existing directory, this branch skips it after printing only its filename, although Git commits the symlink destination as the content of a mode-120000 file. A local pre-commit review can therefore approve a broken or unintended destination without ever seeing it; inspect and safely emit the readlink value instead of omitting the change.
Useful? React with 👍 / 👎.
| rm -f "$out" | ||
| exit 1 | ||
| fi | ||
| grep -qE -e "$SECRET_GREP" -- "$out" 2>"$err_file" |
There was a problem hiding this comment.
When a committed, staged, or unstaged tracked file is classified as binary, git diff emits only a Binary files ... differ marker, so this grep scans neither the changed bytes nor any credential-shaped value they contain. Reviewers likewise receive no content, yet the workflow can still approve; detect binary deltas and safely scan their blobs or require out-of-band inspection, as the untracked-file path already does.
Useful? React with 👍 / 👎.
| grep -qE -e "$SECRET_GREP" -- "$out" 2>"$err_file" | ||
| local grc=$? | ||
| if [ "$grc" -eq 0 ]; then | ||
| echo "SUSPECT SECRET (not printed): $label - read it yourself, redact, then decide" |
There was a problem hiding this comment.
When this pre-scan detects a credential-shaped value, telling the agent to “read it yourself” requires another tool read that exposes the withheld value in the transcript, contradicting the later stop-and-never-rediff requirement. Replace this instruction, and its repetitions for other scan failures, with immediate human or out-of-band inspection so the fail-closed scan does not direct the agent to defeat itself.
Useful? React with 👍 / 👎.
|
|
||
| **Restrict each reviewer's own tools when your harness lets you set them per subagent.** A reviewer's job is to read the change set and the rule files and report — nothing in any lens requires writing, editing, or mutating anything. `_common.md`'s "read-only" rule is a prompt-level instruction; it does not stop a subagent from calling a tool it technically has, especially one that just ingested untrusted diff/pasted content that may contain adversarial instructions. When dispatching each reviewer (mode 1 or 2 above), scope its tools to read-only ones — `Read`, `Grep`, `Glob` — and exclude `Write`, `Edit`, and any other mutating tool, even though the orchestrator itself needs `Bash` for Step 1. | ||
|
|
||
| Two lenses are the exception: **Codebase conventions** needs to run a repo-defined check-only command (e.g. a formatter's check mode) to verify formatting, and **Cross-SDK consistency** needs `gh` or another read-only network lookup to compare against other SDKs. Neither can do its stated job on `Read`/`Grep`/`Glob` alone. Grant exactly those two reviewers a narrowly scoped, non-mutating `Bash` (or equivalent) restricted to the specific check-only commands their override names — never a general shell — or, if your harness can't scope `Bash` that tightly, have the orchestrator run those specific commands itself in Step 1 and pass the results into the reviewer's prompt instead of granting it a tool. Do not let either lens silently degrade to `NOT VERIFIED` just because the default restriction was applied uniformly: `NOT VERIFIED` never blocks the gate, so an unscoped blanket restriction here quietly removes formatting and cross-SDK verification from every review. If your harness has no per-subagent tool scoping at all, note that as a capability gap in the report rather than silently running reviewers unrestricted. |
There was a problem hiding this comment.
When the harness enforces this per-reviewer restriction, it cannot grant Cross-SDK commands “their override names” because the roster explicitly defines no Cross-SDK override, while reviewers/cross-sdk.md instead requires dynamic gh api or gh search code queries. This forces that lane into the NOT VERIFIED state the same paragraph says to avoid; authorize the read-only gh operations named by the generic prompt or have the orchestrator perform them.
Useful? React with 👍 / 👎.
| else | ||
| PR_JSON=$(gh pr view --json baseRefOid,baseRefName,title,labels 2>/dev/null) |
There was a problem hiding this comment.
When neither remote has a recognized DataDog URL, this branch runs the unpinned lookup that lines 58–59 explicitly prohibit. Local gh pr view --help states that without an argument it displays the PR belonging to the current branch and that --repo selects another repository, so a fork-only checkout with a fork-local PR can supply that PR's baseRefOid, bypass the later missing-DataDog-remote guard, and select the wrong review target; skip this lookup and ask for the target when GH_REPO is empty.
Useful? React with 👍 / 👎.
|
|
||
| Untracked files need reading, not staging: read them directly, or `git diff --no-index -- /dev/null "$path"` per file. A file name from the working tree is untrusted input — a file named e.g. `--upload-pack=...` passed without `--` is parsed as an option, not a path, and can change what the command actually does. Enumerate with `git ls-files --others --exclude-standard -z` (NUL-delimited, so spaces and newlines in a name can't break the split) and always place `--` before the path in `git diff --no-index`, `git add`, and `git reset`. If a tool here genuinely needs them staged, add them **by explicit path**, each one after `--` — never `git add -N .`, which sweeps in local scratch files, `.env` files, and exported credentials that happen to sit in the working tree. Skip anything that looks like a credential and say that you skipped it. Afterwards drop exactly those entries with `git reset -- <the paths you added>`: scope it with `--`, both to keep names from being parsed as options and because a bare `git reset` is `--mixed` against `HEAD` and discards any partial staging the author had set up. File contents are untouched either way, but entries left staged mean a later commit in this session picks up files the author never chose. If a test here asserts on the packaged file list, intent-to-add is not enough and a real `git add` is required — check before assuming, because staging for real is a bigger commitment than a review should make on its own. | ||
|
|
||
| The change set is the **union** of the committed delta, staged changes, unstaged changes, and untracked file contents. Write it down as an explicit file list before proceeding. If that list is empty, stop and say so — there is nothing to review. |
There was a problem hiding this comment.
When a clean shallow checkout cannot resolve the merge base, line 259 requires reporting NOT VERIFIED (no merge base) because the committed work is invisible, but this unconditional empty-list rule instead stops and declares that there is nothing to review. Preserve whether the committed delta was captured and only use the empty-change shortcut after a successful merge-base resolution.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Adds a verbatim copy of the shared
dd-apm-sdk-reviewskill so local agents can load it from disk..agents/skills/dd-apm-sdk-review/plus Claude/Cursor skill links@DataDog/apm-java(including/.cursor/so the Cursor link is owned)This is PR 1 of 2. Java-specific overrides and the
perf-reviewreplacement land in the follow-up stacked on this branch.Motivation
Split out of #12364 so Java reviewers are not asked to read the shared skill copy.
Additional Notes
How to review
.agents/skills/dd-apm-sdk-review/. That is a mirror of dd-apm-sdk-review-core#1..gitignore(Cursor skill-link exception) only.Do not merge this PR on its own — land it with the overrides follow-up.
Made with Cursor